-
Notifications
You must be signed in to change notification settings - Fork 171
[4기 - 김민희] SpringBoot Part3 WeeklyMission 제출합니다. #867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: KimMinheee/week01
Are you sure you want to change the base?
[4기 - 김민희] SpringBoot Part3 WeeklyMission 제출합니다. #867
Conversation
- mapStruct - spring-boot-starter-seb
- ErrorCode 내에서 관리하도록 수정
- Mapper 사용 - Controller Dto @Valid로 유효성 확인
- memberUUID -> memberId로 변경
- created_at 필드 구현
- created_at 컬럼 추가
- 매개변수로 value 값 전달받도록 수정
import java.time.LocalDateTime; | ||
import java.time.format.DateTimeFormatter; | ||
|
||
@Component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 entity는 Component가 아닌것 같아요
*/ | ||
@ExceptionHandler(MethodArgumentNotValidException.class) | ||
protected ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) { | ||
log.error("Handle MothodArgumentNotValidException", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRACE, DEBUG, INFO, WARN, ERROR 등
로그의 레벨이 나뉘어져 있어요.
각각 나뉜 의도가 있습니다.
MethodArgumentNotValidExceptionr가 발생하면 error일까요?
그렇다면 이 로그는 error일까요?
@RequiredArgsConstructor | ||
@RestControllerAdvice | ||
@Slf4j | ||
public class GlobalExceptionHandler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름좋은데요!
Rest Controller에 관련된 예외를 처리하는 handler다 라고 조금 더 명확하게 주면 어떨까요?
protected ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e) { | ||
log.error("Handle MothodArgumentNotValidException", e.getMessage()); | ||
final ErrorResponse response = ErrorResponse.of(ErrorCode.INVALID_METHOD_ERROR, e.getBindingResult()); | ||
return new ResponseEntity<>(response, HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity.badRequest().body() 라는 정적 팩토리 메소드가 존재합니다.
protected ResponseEntity<ErrorResponse> handleJsonProcessingException(JsonProcessingException ex) { | ||
log.error("handleJsonProcessingException", ex); | ||
final ErrorResponse response = ErrorResponse.of(ErrorCode.REQUEST_BODY_MISSING_ERROR, ex.getMessage()); | ||
return new ResponseEntity<>(response, HttpStatus.OK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseEntity.ok()
|
||
return switch (discountType) { | ||
case FIXED -> | ||
new FixedAmountVoucher(UUID.randomUUID(), discountType, new DiscountValue(voucherCreateRequest.discountValue())); | ||
new FixedAmountVoucher(UlidCreator.getUlid().toString(), discountType, new DiscountValue(voucherCreateRequest.discountValue())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 static 메소드에 직접 의존하면 테스트하기도 어려울뿐더러
지금처럼 UUID -> ULID로 바뀌었을때도 상당히 많은 부분이 바뀌어야 하네요.
어떻게 해결할 수 있을까요?
* @return BaseResponse<VoucherCreateResponse> | ||
*/ | ||
@PostMapping() | ||
public BaseResponse<VoucherCreateResponseData> createVoucher(@Valid @RequestBody VoucherCreateRequestData data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestData 라는 말이 저는 중복같아요보여요
Data 또는 Request 하나만 써도 될 것 같은데 어떻게 생각하세요?
* @param data | ||
* @return BaseResponse<String> | ||
*/ | ||
@PatchMapping("/{voucherId}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PATCH와 PUT는 어떤 차이점이 있을까요?
@NotBlank | ||
String discountType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum으로도 받을 수 있습니다.
.orElseThrow(() -> new MemberException(NOT_FOUND_MEMBER)); | ||
|
||
Wallet wallet = new Wallet(UUID.randomUUID(), | ||
Wallet wallet = new Wallet(UlidCreator.getUlid().toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
직접의존!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
민희님 바우처 과제 하시느라 수고 많으셨습니다. 추후 다른 과제에서도 지금처럼 좋은 모습 보여주세요 ㅎㅎ
성실하게 마무리해주셔서 감사합니다.
final ErrorResponse response = ErrorResponse.of(ErrorCode.INTERNAL_SERVER_ERROR, e.getMessage()); | ||
return new ResponseEntity<>(response, HttpStatus.OK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception은 internal server error가 맞는 것 같아요.
@ExceptionHandler(MemberException.class) | ||
public ResponseEntity<ErrorResponse> handleNotFoundException(MemberException e) { | ||
log.error("Handle NotFoundException :", e); | ||
final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage()); | ||
return new ResponseEntity<>(response, HttpStatus.OK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404 혹은 400이 맞는 에러코드인 것 같습니다. 멤버를 못찾는 요청이 잘못된 경우(400), 찾는 멤버가 없는 경우(404)
@ExceptionHandler(VoucherException.class) | ||
public ResponseEntity<ErrorResponse> handlePermissionDeniedException(VoucherException e) { | ||
log.error("Handle PermissionDeniedException :", e); | ||
final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage()); | ||
return new ResponseEntity<>(response, HttpStatus.OK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permission denied에 맞는 http status 를 알아보시면 좋을 것 같네요. 아래도 마찬가지고요.
인증과 인가 두 개념에 대해서 http status 값에 맞는 경우를 탐구해보면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹은 단순히 400으로 볼 수도 있고요. 순전히 요청이 잘못됐다고 서버가 판단해야하는 포괄적인 경우 http status 에 의해서 비즈니스가 노출될 수 있으니깐요.
@ExceptionHandler(WalletException.class) | ||
public ResponseEntity<ErrorResponse> handlePermissionDeniedException(WalletException e) { | ||
log.error("Handle PermissionDeniedException :", e); | ||
final ErrorResponse response = ErrorResponse.of(e.getErrorCode(), e.getMessage()); | ||
return new ResponseEntity<>(response, HttpStatus.OK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permission denied에 맞는 http status 를 알아보시면 좋을 것 같네요. 아래도 마찬가지고요.
인증과 인가 두 개념에 대해서 http status 값에 맞는 경우를 탐구해보면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹은 단순히 400으로 볼 수도 있고요. 순전히 요청이 잘못됐다고 서버가 판단해야하는 포괄적인 경우 http status 에 의해서 비즈니스가 노출될 수 있으니깐요.
@@ -23,33 +22,34 @@ public JdbcMemberReaderRepository(JdbcTemplate jdbcTemplate) { | |||
|
|||
@Override | |||
public List<Member> findAll() { | |||
String sql = "select member_id, member_status, name from member_table"; | |||
String sql = "SELECT member_id, member_status, name FROM member_table"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비슷한 의견으로 member 테이블의 id인데 굳이 member_id
라는 명명이 되어야 하는지도요~
@GetMapping("/voucher/{voucherId}") | ||
public BaseResponse<WalletGetResponses> getWalletsByVoucherId(@PathVariable String voucherId) { | ||
WalletGetResponses responses = walletService.getWalletsByVoucherId(voucherId); | ||
return new BaseResponse<>(responses); | ||
} | ||
|
||
|
||
@GetMapping("/member/{memberId}") | ||
public BaseResponse<WalletGetResponses> getWalletsByMemberId(@PathVariable String memberId) { | ||
WalletGetResponses responses = walletService.getWalletsByMemberId(memberId); | ||
return new BaseResponse<>(responses); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지갑 목록에서 바우처 아이디
, 멤버 아이디
두 파라미터를 받아 검색하는 지갑 목록 api로도 작성해볼 수 있겠네요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 지금도 좋긴합니다. 오히려 ocp에는 굉장히 만족스러운 행위입니다. 다만 처음에 개발할 당시엔 공통되는 부분은 최대한 로직의 재사용을 할 수 있도록 해주면 좋아요.
📌 과제 설명
👩💻 요구 사항과 구현 내용
VoucherManagement Rest API 개발
- ExceptionHandler 이용
- BaseResponse를 이용한 응답 데이터 형식 통일화
2주차 이후 수정한 내용
- Voucher, Member, Wallet의 id값을 UUID -> ULID로 수정
- DTO를 계층별로 구분 및 Mapper 이용
- SQL 쿼리 키워드 대문자로 수정
- BaseTimeEntity를 이용하여 created_at 데이터 추가